Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Handle failure in ffi_closure_alloc #1378

Merged
merged 1 commit into from Sep 14, 2021
Merged

Handle failure in ffi_closure_alloc #1378

merged 1 commit into from Sep 14, 2021

Conversation

DaveCTurner
Copy link
Contributor

@DaveCTurner DaveCTurner commented Aug 30, 2021 •
edited

ffi_closure_alloc may fail and return NULL if, for instance, we're
running in a locked-down operating system that forbids FFI from
allocating executable pages of memory in any of the ways that it tries.
Today we pass this NULL on to ffi_prep_closure_loc which triggers a
segmentation fault that takes down the whole JVM. With this change we
check for a failure in this call and turn it into an
UnsupportedOperationException so that the caller can handle it more
gracefully.

Relates elastic/elasticsearch#73309
Relates elastic/elasticsearch#18272
Closes #1107

@DaveCTurner
Copy link
Contributor Author

I had a bit of a look for tests of similar things but didn't see anything obvious, nor can I think of good ways to test this change. Some guidance would be appreciated.

@dkocher
Copy link
Contributor

dkocher commented Aug 30, 2021

Looks like a fix for #1107.

@DaveCTurner DaveCTurner mentioned this pull request Aug 30, 2021
Closed
@matthiasblaesing
Copy link
Member

Change looks good to me. Thank you.

I agree testing this will be difficult to impossible. You could startup a virtual machine, lock that down, test to run JNA in that setup and observe what happens. I think that the balance between effort and results would tip in a bad way, so I think review is enough here.

Could please also have a look at:

jna/native/dispatch.c

Lines 3515 to 3535 in a40db1b

JNIEXPORT jlong JNICALL
Java_com_sun_jna_Native_ffi_1prep_1closure(JNIEnv *env, jclass UNUSED(cls), jlong cif, jobject obj)
{
callback* cb = (callback *)malloc(sizeof(callback));
ffi_status s;
if ((*env)->GetJavaVM(env, &cb->vm) != JNI_OK) {
throwByName(env, EUnsatisfiedLink, "Can't get Java VM");
return 0;
}
cb->object = (*env)->NewWeakGlobalRef(env, obj);
cb->closure = ffi_closure_alloc(sizeof(ffi_closure), L2A(&cb->x_closure));
s = ffi_prep_closure_loc(cb->closure, L2A(cif), &closure_handler,
cb, cb->x_closure);
if (ffi_error(env, "ffi_prep_cif", s)) {
return 0;
}
return A2L(cb);
}

Lines 3527 and 3529 look pretty similar, so I think a guard should be placed there also.

@DaveCTurner
Copy link
Contributor Author

Lines 3527 and 3529 look pretty similar, so I think a guard should be placed there also.

Sure, I think bb75249 does the right thing here.

@matthiasblaesing
Copy link
Member

Eyeballed this and looks good to me. To get this in please squash the changes into a single commit. After merge I'll see which architectures I manage to rebuild.

@DaveCTurner
Handle failure in ffi_closure_alloc
3f5e937 
`ffi_closure_alloc` may fail and return `NULL` if, for instance, we're
running in a locked-down operating system that forbids FFI from
allocating executable pages of memory in any of the ways that it tries.
Today we pass this `NULL` on to `ffi_prep_closure_loc` which triggers a
segmentation fault that takes down the whole JVM. With this change we
check for a failure in this call and turn it into an
`UnsupportedOperationException` so that the caller can handle it more
gracefully.

Relates elastic/elasticsearch#73309
Relates elastic/elasticsearch#18272
@DaveCTurner DaveCTurner force-pushed the 2021-08-30-ffi_closure_alloc-returns-NULL branch from 5299bd5 to 3f5e937 Compare September 13, 2021 07:59
@DaveCTurner
Copy link
Contributor Author

Sure thing. The squashed commit is 3f5e937.

@matthiasblaesing matthiasblaesing merged commit 4ebb64a into java-native-access:master Sep 14, 2021
@matthiasblaesing
Copy link
Member

Thank you.

@DaveCTurner DaveCTurner mentioned this pull request Sep 21, 2021
Closed
@DaveCTurner DaveCTurner deleted the 2021-08-30-ffi_closure_alloc-returns-NULL branch September 21, 2021 06:58
@dkocher dkocher mentioned this pull request Oct 2, 2021
Closed
@schlosna schlosna mentioned this pull request Sep 14, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dbwiddis dbwiddis dbwiddis left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Segfault due to unchecked return value
4 participants
@DaveCTurner @dkocher @matthiasblaesing @dbwiddis